Skip to content

Review#1

Open
Solnec wants to merge 2 commits intomasterfrom
review
Open

Review#1
Solnec wants to merge 2 commits intomasterfrom
review

Conversation

@Solnec
Copy link
Copy Markdown
Owner

@Solnec Solnec commented Oct 13, 2022

No description provided.

//-------------------------------------------------------------//
void SingletonGlobalVars::NewRound(AddressesList& addressesList, bool& newRound)
{
int newMatch = *(int*)(addressesList.inMatch);
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему бы не хранить сразу int* в AddressesList, все равно каждый раз приводишь к int* и точно ли есть необходимость в арифметике указателей?

}

//-------------------------------------------------------------//
void SingletonGlobalVars::FindMaxId(const int maxPlayers, AddressesList& addressesList, int maxId)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вроде бы нет необходимости в out параметре обычное нахождение максимума


//names attacker
if (ents.size())
for (std::shared_ptr<Ent>& player : ents)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

в range based циклах можно опускать полное имя типа для простоты

constexpr float kCrouchAdjustment = 3.0f;
constexpr float kOnGround = 2.0f;
constexpr float kCrouchHeightAdjustment = -35.0f;
constexpr float kDegreesToRad = PI / 180.0f;
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Можно использовать стандартные средства языка


bool Aim::IsOnGround(int stance) const
{
bool isFallen = false;
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ненужная переменная, просто возвращай true/false

{
bool isFallen = false;
if (stance < 50
|| stance == 1234
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic numbers лучше чтобы все константы были названы. Например помести эти числа в enum

if (((X < 0) && (Y > 0)) || ((X < 0) && (Y < 0)))
{
angle = atan(Y / X);
angle += (float)PI;
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

С-style cast почти всегда плохая идея, он незаметный в коде и может ломать код. Если очень надо, то лучше использовать reinterpret_cast но обычно такая необходимость значит, что есть ошибка в архитектуре

angle = (float)(2 * PI + atan(Y / X));
angle = 2 * std::numbers::pi_v<float> + atan(Y / X);
else
angle = (float)atan(Y / X);
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тут atan итак возвращает float

}

//-------------------------------------------------------------//
void Aim::Pblocks(std::shared_ptr<Ent> &enemy)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Передавать умный указатель по ссылке нужно только осознавая, что в таком случае счетчик ссылок может обнулиться в неподходящий момент, и ссылка перестанет ссылаться на валидные данные. В данном случае вроде бы можно, но тогда по константной ссылке, все равно только чтение

}

//-------------------------------------------------------------//
std::shared_ptr<Ent> Aim::ChooseTarget(std::shared_ptr<Ent> &chosenTarget, const bool &isTk, bool &isAim)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

зачем константная ссылка на бул? Дешевле скопировать 1 байт чем разрядность_системы_бит. Обычно бул передают по значению


///////////////////////////////////////////////////////////////
// class Ent - entity (player)
class Ent
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Названия классов лучше не сокращать, улучшается читабельность

int id;
int surface;
int saberAnim;
int numWeapon;
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Количество оружия не может быть отрицательным -> лучше использовать беззнаковый инт. Индексы массива тоже всегда неотрицательны(но тут принято size_t) и т.д. если можно использовать более "узкий" тип данных лучше использовать его, поможет избежать случайных ошибок и повысит читабельность

bool correctColor3;
bool correctColor4;

const char* className;
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачем дублировать имя?

</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
<LinkIncremental>true</LinkIncremental>
<OutDir>C:\Users\ооррр\Desktop\epicHax</OutDir>
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Пути в проекте лучше делать относительными, а не абсолютными, например у меня такой папки не было на компьютере

<ConformanceMode>true</ConformanceMode>
<PrecompiledHeader>Use</PrecompiledHeader>
<PrecompiledHeaderFile>pch.h</PrecompiledHeaderFile>
<LanguageStandard>stdcpp20</LanguageStandard>
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вроде в readme было написано что 20ый стандарт используется, а по факту стоял 14ый(дефолтьный)


//-------------------------------------------------------------//
void VectorSubtract(const vec3_t vec1, const vec3_t vec2, vec3_t vecOut)
void VectorSubtract(const vec3_t& vec1, const vec3_t& vec2, vec3_t& vecOut)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.Выходной параметр обязательно должен передаваться по ссылке, иначе создается копия, которая в последствии и изменяется внутри метода и при выходе из скоупа удаляется
2.vec1 и vec2 тоже желательно передавать по константной ссылке(констаность была правильной уже), а по ссылке чтобы не копировать

}

//-------------------------------------------------------------//
void VecDivByNum(vec3_t in, float numDiv)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лучше назвать более нейтрально так как по сути это in_out параметр и по ссылке конечно

@@ -1,8 +1,4 @@
#pragma once
#include <cmath>
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В header-ы лучше не подключать лишних библиотек или заголовочных файлов, потому что ты их потом везде подключаешь и это раздувает объектные файлы и порождает циклические зависимости.


constexpr float PI = 3.14159265358979323846;

typedef float vec3_t[3];
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вместо множества свободных функций лучше сделать полноценный класс 3д вектора с переопределенными операторами т .д. . В коде это будет выглядеть как v1 - v2 и т.д.
Как минимум поместить в отдельный namespace т.к. засорять глобальную область видимости лучше не надо

unsigned int numWeapon;
int stance;
int stance2;
int hp;
Copy link
Copy Markdown
Owner Author

@Solnec Solnec Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В производном классе не нужен член myHp, зачем дублировать уже унаследованный челн класса?
Слишком много полей в одном классе, слишком много на себя берет, нужно разбивать на иерархию скорее всего или на отдельные модули которые будут взаимодействовать между собой.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant